Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Allow a percent_max to be specified in ingredients Fixes #5369 #7639

Merged
merged 12 commits into from
Nov 9, 2022
Merged

feat: Allow a percent_max to be specified in ingredients Fixes #5369 #7639

merged 12 commits into from
Nov 9, 2022

Conversation

john-gom
Copy link
Contributor

  • Code passes GitHub workflow checks in your branch
    Getting a problem with perltidy on Windows:
$ make check_perltidy
🥫 Checking with perltidy lib/ProductOpener/Ingredients.pm tests/unit/ingredients_percent.t
test -z "lib/ProductOpener/Ingredients.pm tests/unit/ingredients_percent.t" || docker-compose --env-file=.env run --rm --no-deps backend perltidy --assert-tidy -opath=/tmp/ --standard-error-output lib/ProductOpener/Ingredients.pm tests/unit/ingredients_percent.t
unable to create directory C:/Users/thego/AppData/Local/Temp/: No such file or directory
make: *** [check_perltidy] Error 1

What

Add a percent_max property to ingredients taxonomy so for ingredients that are never more than a certain percentage, unless they are the main ingredient

Related issue(s) and discussion

@john-gom john-gom requested a review from a team as a code owner October 31, 2022 18:48
@github-actions github-actions bot added 🥗 Ingredients 🧬 Taxonomies https://wiki.openfoodfacts.org/Global_taxonomies 🧬 Taxonomies - Rebuild Taxonomies are compiled before they can be used. 🧪 tests labels Oct 31, 2022
@john-gom john-gom changed the title feat:Allow a percent_max to be specified in ingredients Fixes #5369 feat: Allow a percent_max to be specified in ingredients Fixes #5369 Oct 31, 2022
stephanegigandet
stephanegigandet reviewed Nov 2, 2022
View reviewed changes
lib/ProductOpener/Ingredients.pm Outdated
@@ -2532,9 +2533,16 @@ sub init_percent_values ($total_min, $total_max, $ingredients_ref) {
$ingredient_ref->{percent_min} = 0;
}
if ((not defined $ingredient_ref->{percent_max}) or ($ingredient_ref->{percent_max} > $total_max)) {
$ingredient_ref->{percent_max} = $total_max;
my $value = get_inherited_property("ingredients", $ingredient_ref->{id}, "percent_max:en");
if (defined $value and $index > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not construct a test case where it made a difference, but I'd feel safer if we also tested that $value < $total_max

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case although it didn't fail without the guard, but added the guard too anyway

stephanegigandet
stephanegigandet reviewed Nov 2, 2022
View reviewed changes
Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @john-gom , thanks a lot for the PR. Looks good to me, I just have 2 minor suggestions.

stephanegigandet
stephanegigandet approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@stephanegigandet stephanegigandet mentioned this pull request Nov 3, 2022
Open
@github-actions github-actions bot added the 🥗🔍 Ingredients analysis https://wiki.openfoodfacts.org/Ingredients_Extraction_and_Analysis label Nov 7, 2022
@john-gom
Copy link
Contributor Author

john-gom commented Nov 7, 2022

Have made some more changes to limit the "blast radius" of this change, so percent_max will not be applied in the following scenarios:

  1. To sub-ingredients
  2. Where the first ingredient has a percent_max defined
  3. If the percent_max would be less than any later (smaller quantity) ingredients
  4. If the larger ingredients have percentages specified and the percent max when added this would not allow the total to reach 100%

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

stephanegigandet
stephanegigandet approved these changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@stephanegigandet stephanegigandet merged commit e01e83f into openfoodfacts:main Nov 9, 2022
@alexgarel alexgarel mentioned this pull request Nov 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanegigandet stephanegigandet stephanegigandet approved these changes

Assignees

@john-gom john-gom

Labels
🥗🔍 Ingredients analysis https://wiki.openfoodfacts.org/Ingredients_Extraction_and_Analysis 🥗 Ingredients 🧬 Taxonomies - Rebuild Taxonomies are compiled before they can be used. 🧬 Taxonomies https://wiki.openfoodfacts.org/Global_taxonomies 🧪 tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Limit percent max of some ingredients (e.g. aroma, ferments) (needed for milk estimate for Nutri-Score)
2 participants
@john-gom @stephanegigandet